APIScan | Fix Builder Mistake #3357
Conversation
benrr101
added
the
Regression 💥
There was a problem hiding this comment.
Pull Request Overview
This PR corrects the builder pattern in CreateClientAppInstance by ensuring the modified builder is stored back into the builder variable for the Win32 window scenario.
- Assigns the result of
WithParentActivityOrWindowback tobuilder - Prevents the lost configuration when
_iWin32WindowFuncis not null
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs:559
- Add a unit test to verify that when
_iWin32WindowFuncis provided,WithParentActivityOrWindowis actually called and the returned builder is used, ensuring this configuration isn't lost.
builder = builder.WithParentActivityOrWindow(_iWin32WindowFunc);
|
@mdaigle If I had any idea how to do that ... sure |
|
The simplest repro would be a netfx winforms app. Visual studio can create one for you from a template. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3357 +/- ##
==========================================
+ Coverage 65.10% 67.72% +2.61%
==========================================
Files 300 294 -6
Lines 65376 65066 -310
==========================================
+ Hits 42565 44065 +1500
+ Misses 22811 21001 -1810
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (_iWin32WindowFunc is not null) | ||
| { | ||
| builder.WithParentActivityOrWindow(_iWin32WindowFunc); | ||
| builder = builder.WithParentActivityOrWindow(_iWin32WindowFunc); |
There was a problem hiding this comment.
Is this actually necessary? Chaining typically returns the same object the method was called on, not a new object. Pretty sure WithParentActivityOrWindow() is setting internal state on builder and then returning builder. I'd be very surprised if it was returning a different instance of a Builder.
The API docs don't say either way, but the source code returns this:
I think your original change was fine (i.e. it worked) and maybe that's why there were no test failures. This might be slightly more robust, but I doubt MSAL will change its chaining behaviour, and break a bunch of calling code.
There was a problem hiding this comment.
Because config is local to builder creation, I think it is necessary to store the updated config in the builder instance.
You can validate the builder assignment need by updating another config that overrides something else that can be validated easily in a standalone console app, where you can directly work with MSAL APIs for testing the theory.
benrr101
removed
the
Regression 💥
|
Backing PR out since as per discussion w/ @paulmedynski the builder just updates it's internal state and returns itself. No need to store the output of any With* call since the builder is modified internally. |
Description: Made a mistake when I wrote #3354 - forgot to store the modified builder in the builder variable.
Testing: Kinda disappointed it didn't get caught in any CI pathways. Ideally I should write up a test for this specific scenario, but I'm concerned this will be a huge undertaking versus fixing it before it gets released.